Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIA-R8: add type="password" (and more) to applicability #1667

Merged
merged 32 commits into from
Sep 17, 2024

Conversation

dan-tripp-siteimprove
Copy link
Contributor

@dan-tripp-siteimprove dan-tripp-siteimprove commented Aug 13, 2024

This PR was motivated by the fact that I want to add sia-r8 to the SMART browser extension, and I want it to have applicability which covers <input type="password"> elements, and I don't want to copy-and-paste alfa code into SMART in order to accomplish that.

And of course, I hope that it's useful to alfa users in general.

My only current concrete need in SMART is for type="password", but this PR also contains a few more types: color, date, datetime-local, file, month, time, and week. I got this list from Mark's comment here.

@dan-tripp-siteimprove dan-tripp-siteimprove requested a review from a team as a code owner August 13, 2024 00:25
Copy link

changeset-bot bot commented Aug 13, 2024

🦋 Changeset detected

Latest commit: f37faf4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 77 packages
Name Type
@siteimprove/alfa-rules Minor
@siteimprove/alfa-act Minor
@siteimprove/alfa-affine Minor
@siteimprove/alfa-applicative Minor
@siteimprove/alfa-aria Minor
@siteimprove/alfa-array Minor
@siteimprove/alfa-bits Minor
@siteimprove/alfa-branched Minor
@siteimprove/alfa-cache Minor
@siteimprove/alfa-callback Minor
@siteimprove/alfa-cascade Minor
@siteimprove/alfa-clone Minor
@siteimprove/alfa-collection Minor
@siteimprove/alfa-comparable Minor
@siteimprove/alfa-compatibility Minor
@siteimprove/alfa-continuation Minor
@siteimprove/alfa-css-feature Minor
@siteimprove/alfa-css Minor
@siteimprove/alfa-device Minor
@siteimprove/alfa-dom Minor
@siteimprove/alfa-earl Minor
@siteimprove/alfa-either Minor
@siteimprove/alfa-emitter Minor
@siteimprove/alfa-encoding Minor
@siteimprove/alfa-equatable Minor
@siteimprove/alfa-flags Minor
@siteimprove/alfa-fnv Minor
@siteimprove/alfa-foldable Minor
@siteimprove/alfa-functor Minor
@siteimprove/alfa-future Minor
@siteimprove/alfa-generator Minor
@siteimprove/alfa-graph Minor
@siteimprove/alfa-hash Minor
@siteimprove/alfa-http Minor
@siteimprove/alfa-iana Minor
@siteimprove/alfa-iterable Minor
@siteimprove/alfa-json-ld Minor
@siteimprove/alfa-json Minor
@siteimprove/alfa-lazy Minor
@siteimprove/alfa-list Minor
@siteimprove/alfa-map Minor
@siteimprove/alfa-mapper Minor
@siteimprove/alfa-math Minor
@siteimprove/alfa-monad Minor
@siteimprove/alfa-network Minor
@siteimprove/alfa-option Minor
@siteimprove/alfa-parser Minor
@siteimprove/alfa-performance Minor
@siteimprove/alfa-predicate Minor
@siteimprove/alfa-promise Minor
@siteimprove/alfa-record Minor
@siteimprove/alfa-rectangle Minor
@siteimprove/alfa-reducer Minor
@siteimprove/alfa-refinement Minor
@siteimprove/alfa-result Minor
@siteimprove/alfa-sarif Minor
@siteimprove/alfa-selective Minor
@siteimprove/alfa-selector Minor
@siteimprove/alfa-sequence Minor
@siteimprove/alfa-set Minor
@siteimprove/alfa-slice Minor
@siteimprove/alfa-string Minor
@siteimprove/alfa-style Minor
@siteimprove/alfa-table Minor
@siteimprove/alfa-test Minor
@siteimprove/alfa-thenable Minor
@siteimprove/alfa-thunk Minor
@siteimprove/alfa-time Minor
@siteimprove/alfa-toolchain Minor
@siteimprove/alfa-trampoline Minor
@siteimprove/alfa-tree Minor
@siteimprove/alfa-trilean Minor
@siteimprove/alfa-tuple Minor
@siteimprove/alfa-url Minor
@siteimprove/alfa-wcag Minor
@siteimprove/alfa-web Minor
@siteimprove/alfa-xpath Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this will require changes in Dory and SI platform (and extension), I think it should be a new version of the rule:

  • Create a new sia-er8 directory with this file, same for the tests.
  • Add a Version tag to the tags array of the rule, with version 2.
  • Add the rule to the list of experimental rules.
  • Update changeset accordingly.

We'll need to manage it on our side to be able to handle it. From SMART extension, you'll be able to replace R8 by ER8 in the list of rules you run.

packages/alfa-rules/src/sia-r8/rule.ts Outdated Show resolved Hide resolved
packages/alfa-rules/src/sia-r8/rule.ts Outdated Show resolved Hide resolved
packages/alfa-rules/src/sia-r8/rule.ts Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
packages/alfa-rules/test/sia-r8/rule.spec.tsx Outdated Show resolved Hide resolved
@dan-tripp-siteimprove
Copy link
Contributor Author

@Jym77 there is a new problem that I don't understand, and I don't know if it's a real problem or not: a certain github action failed with this message:

Warning: You have changed the public API signature for this project. Please copy the file "/home/runner/work/alfa/alfa/docs/review/api/compare/alfa-dom.api.md" to "/home/runner/work/alfa/alfa/docs/review/api/alfa-dom.api.md", or perform a local build (which does this automatically). See the Git repo documentation for more info."

So I tried some things, but nothing was fruitful. Any thoughts?

@Jym77
Copy link
Contributor

Jym77 commented Sep 2, 2024

@Jym77 there is a new problem that I don't understand, and I don't know if it's a real problem or not: a certain github action failed with this message:

Warning: You have changed the public API signature for this project. Please copy the file "/home/runner/work/alfa/alfa/docs/review/api/compare/alfa-dom.api.md" to "/home/runner/work/alfa/alfa/docs/review/api/alfa-dom.api.md", or perform a local build (which does this automatically). See the Git repo documentation for more info."

So I tried some things, but nothing was fruitful. Any thoughts?

You need to update the API extraction (which is then used to build some documentation at release time) by running locally yarn extract (or yarn extract packages/alfa-dom to avoid running it on all packages) and committing the resulting file.
Alternatively, we have a handy "PR command" integration, so you just need to comment !pr extract to let Github do the magic (I'll do that in the next comment). The magic happens through PR command action and its configuration file.

@Jym77
Copy link
Contributor

Jym77 commented Sep 2, 2024

!pr extract

@dan-tripp-siteimprove
Copy link
Contributor Author

@Jym77 Excellent. I've noted this "pr extract" and this PR in general for the future, because it contains a lot of generic advice and alfa conventions. At any rate, I think that I've acted on all of the points in your review. What should I do next?

Copy link
Contributor

@Jym77 Jym77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Just a bit more cleanup and polishing.

.changeset/swift-meals-check.md Outdated Show resolved Hide resolved
packages/alfa-rules/src/experimental.ts Outdated Show resolved Hide resolved
packages/alfa-rules/src/sia-er8/rule.ts Outdated Show resolved Hide resolved
packages/alfa-rules/src/sia-er8/rule.ts Outdated Show resolved Hide resolved
@dan-tripp-siteimprove
Copy link
Contributor Author

@Jym77 Thank you - not only for this PR review for the immediate goal, but also for the future reference on alfa conventions that this PR review will serve as (the next time I make a PR).

I've "resolved" everything again so it's over to you, again.

@dan-tripp-siteimprove
Copy link
Contributor Author

!pr extract

@Jym77 Jym77 added this pull request to the merge queue Sep 17, 2024
@Jym77
Copy link
Contributor

Jym77 commented Sep 17, 2024

I'm merging this now because I'm going to make conflicting changes, will be easier to start from the new state.

Merged via the queue into Siteimprove:main with commit 15fabab Sep 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants